Persist properties again & unblock thingUpdated() when called from within initialize() #4688
Conversation
…Updated() from BaseThingHandler#updateProperties and #updateProperty (eclipse-archived#1682)" ...now that the framework handles non-managed things gracefully (see eclipse-archived#2629). reverts eclipse-archived#1682 Signed-off-by: Simon Kaufmann <simon.kfm@googlemail.com>
Signed-off-by: Simon Kaufmann <simon.kfm@googlemail.com>
Signed-off-by: Simon Kaufmann <simon.kfm@googlemail.com>
Signed-off-by: Simon Kaufmann <simon.kfm@googlemail.com>
Are you sure, that we will not see a lot of IAE because of the change in updateThing? |
Yes, pretty much. A quick search on eclipse/smarthome and openhab/openhab2-addons revealed only the Hue binding to misbehave in that respect. |
|
||
if (oldThing != thing && oldThing != null) { | ||
private void replaceThing(Thing oldThing, Thing newThing) { | ||
final ThingHandler thingHandler = thingHandlers.get(newThing.getUID()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there is nothing to do if oldThing == newThing
, why not moving this line into your if branch, so it is not executed if not necessary?
try { | ||
lock1.lock(); | ||
replaceThing(getThing(thingUID), thing); | ||
ThingHandler thingHandler = thingHandlers.get(thing.getUID()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
replaceThing
is a private method and access the new thing handler already.
So, if your replaceThing
returns the thing handler of the second argument, there is no need to do it twice.
Signed-off-by: Simon Kaufmann <simon.kfm@googlemail.com>
@maggu2810 both your comments are valid of course, but you cannot have it all 😉 PR is updated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, thanks!
I am seeing similar behavior with the zwave binding when updating the Thing config. Was scratching my head why PaperUI would hang for so long (5 sec) on a simple config update... The specific use case is when changing the temp scale of a channel. Edit: I'm on OH build 1115. @SJKA Do you believe this is the same root cause that will be solved by this PR?
|
I loaded up OH build 1118 (which includes this PR) and the above problem no longer occurs. |
This pull request has been mentioned on openHAB Community. There might be relevant details there: https://community.openhab.org/t/blacklist-troubleshooting/37055/5 |
This PR reverts #1682 as it doesn't hurt anymore to try persisting properties now since #2629 made the framework handle non-managed things gracefully.
In order to make this work it also resolves a dead-lock: While #3351 introduced a lock in order to prevent that in a binding
ThingHandler.thingUpdated()
can be called in parallel withThingHandler.initialize()
from the framework, this mechanism led to a 5s dead-lock if a thing handler actually updates the thing itself during initialization as in #4375 & #4666.Also it disallows bindings to call
BaseThingHandler.updateThing(...)
with the current thing instance by throwing an IAE as mutations should be done viaBaseThingHandler.editThing()
.fixes #4375
fixes #4666
Signed-off-by: Simon Kaufmann simon.kfm@googlemail.com